-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Implement explicit tail calls in the LLVM backend #138555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
rustbot has assigned @Mark-Simulacrum. Use |
Some changes occurred in compiler/rustc_codegen_ssa This PR modifies cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
With this, I think we can enable drop-order test now: https://github.com/rust-lang/rust/blob/989d72f9abc1b4f43b2233ba0664736dc30c69e3/tests/ui/explicit-tail-calls/drop-order.rs |
Looking at the recent comments to understand the GCC backend implementation issue |
Some changes occurred in compiler/rustc_codegen_gcc |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
r? WaffleLapkin |
@semtexzv thanks for your PR! Do you plan to work on this further? As @workingjubilee said ~3 weeks ago, the test you've added doesn't actually test anything. The CI is currently failing. I also have a lot of thoughts on the implementation details. All in all there is still quite a bit of work to be done before this can be merged. Do you have the capacity and desire to continue? I'm a bit concerned that this might be too much for a first time contributor (feel free to prove me wrong tho!) @rustbot author |
Reminder, once the PR becomes ready for a review, use |
@semtexzv |
While I would also be a first time contributor, I would be happy to try and implement this. Though, I would only have time to start work on this in two or three weeks. |
This comment has been minimized.
This comment has been minimized.
I've updated the implementation to use LLVM's Specifics:
Known IssueThe
The test remains disabled (as it was before) pending further investigation. All other tail call tests pass successfully. |
This comment has been minimized.
This comment has been minimized.
rmetaQZcwdq/full.rmeta
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you accidentally committed an rmeta test file
@@ -1181,6 +1192,7 @@ unsafe extern "C" { | |||
pub(crate) safe fn LLVMIsGlobalConstant(GlobalVar: &Value) -> Bool; | |||
pub(crate) safe fn LLVMSetGlobalConstant(GlobalVar: &Value, IsConstant: Bool); | |||
pub(crate) safe fn LLVMSetTailCall(CallInst: &Value, IsTailCall: Bool); | |||
pub(crate) fn LLVMRustSetTailCallKind(CallInst: &Value, Kind: TailCallKind); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one can be made safe
, the caller cannot violate anything that would cause UB
// Call the function | ||
let fn_ty = bx.fn_decl_backend_type(fn_abi); | ||
let fn_attrs = if let Some(instance) = instance | ||
&& bx.tcx().def_kind(instance.def_id()).has_codegen_attrs() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would we ever have a tail call for something that doesn't have codegen attrs?
// We can't test the non-tail version with a large number as it would crash, | ||
// but we can verify it works for small values | ||
assert_eq!(countdown_no_tail(10), 0); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and elsewhere:
nit: missing trailing newline
// Perform the actual function call | ||
let llret = bx.call(fn_ty, fn_attrs, Some(fn_abi), fn_ptr, &llargs, None, instance); | ||
|
||
// Mark as tail call - this is the critical part | ||
bx.set_tail_call(llret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we use normal function codegen and just set_tail_call
at the end?
Can we at least share some of the argument processing code and instance resolution code?
@@ -595,6 +595,10 @@ pub trait BuilderMethods<'a, 'tcx>: | |||
funclet: Option<&Self::Funclet>, | |||
instance: Option<Instance<'tcx>>, | |||
) -> Self::Value; | |||
|
|||
/// Mark a call instruction as a tail call (guaranteed tail call optimization) | |||
/// Used for implementing the `become` expression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All those references to being used to implement become
feel a bit redundant to me.
)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing an sret argument when PassMode::Indirect
is used. Also please deduplicate this code with regular calls where possible to prevent divergence.
Your comments #138555 (comment) and #138555 (comment) are very verbose and hard to process. I don't really have much constructive advice for it
Also your two comments duplicate a lot of information between them. Why are there two comments that partially say the same thing? |
This commit implements proper tail call optimization for the `become` expression by propagating LLVM's musttail attribute, which guarantees tail call optimization rather than leaving it as an optimization hint. Changes: - Add `set_tail_call` method to BuilderMethods trait - Add FFI wrapper LLVMRustSetTailCallKind to access LLVM's setTailCallKind API - Implement tail call handling in LLVM backend using musttail - Implement TailCall terminator codegen in rustc_codegen_ssa - Make GCC backend fail explicitly on tail calls (not yet supported) - Add codegen tests to verify musttail is properly emitted - Add runtime tests for deep recursion and mutual recursion The musttail attribute is critical for the correctness of the `become` expression as it guarantees the tail call will be optimized, preventing stack overflow in recursive scenarios.
The job Click to see the possible cause of the failure (guessed by this bot)
|
Implements guaranteed tail calls in the LLVM backend (#112788):